Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Side navigation #1349

Open
wants to merge 47 commits into
base: main
Choose a base branch
from
Open

Side navigation #1349

wants to merge 47 commits into from

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Jun 30, 2022

Closes #941

  • No hover on open/close button because can figure out how to manage black/white side nav. Open to any ideas ❗ (Tried several on opacity, change the bg-color, transparent seems to be the better way) 🟢 Maybe improve the design by adding a change on color ?
  • Missing [key | click outside] to close a collapsible side nav due to no added Js.
  • Collapsible with content focused item when closed are missing 1px on the right. Could be patched but might increase the bundle size. -> .mw-100.
  • Can't make the scroll on the tabs part only in the scrollable with content. Open to any ideas ❗ Resolved
  • If List group: Branded #1467 is merged before this PR, please try .list-group-item instead of .side-nav-item.

Live preview

Todos

  • Padding top and bottom whatever the number of lines: always the same
  • Add a minimum spacing between the text and the chevron of 10px (shouldn't be feasible until the introduction of flex gap)
  • Spread the selected state to parent if collapsed (example)
  • Make the open/close button 100% width + no hover

@netlify
Copy link

netlify bot commented Jun 30, 2022

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit 2fb3373
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/659671ea3620a80008371650
😎 Deploy Preview https://deploy-preview-1349--boosted.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@louismaximepiton louismaximepiton force-pushed the main-lmp-side-navigation branch from 2b0c2a3 to 2d9ffbf Compare August 3, 2022 08:17
@isabellechanclou
Copy link
Member

isabellechanclou commented Aug 19, 2022

Remains to be done/fixed:

  • On all examples:

    • For each label lines, no matter the tree level, add a truncation + margin/padding right at the end of the line, the 1st one if only one line is allowed or the 2nd one if two are OK. ⚠️ Check with designers was they want on this topic.
      At the moment, we have this behavior:

      image. image image

    • Accordions titles are in bold when they should not.

      image

    • Tooltips:

    • Focus: There is a weird visual behavior when the focus rectangle appears: the vertical borders appear a bit after the horizontal ones. Will be fixed in PR Apply visible focus design guidelines in all components #1437

  • In #basic:

    • Display category items in the same order as in the design specifications
    • Add an example, the variant without accordion icons
  • In #collapsible-with-title:

    • Add a margin/padding between the end of the closed side-navigation menu and the text "test".
    • Replace the text "test" by another text
      image
  • In #collapsible-with-content:

    • There shouldn't have a vertical orange bar on a selected category item when the side navigation menu is open. It might required some js to do that which we want to avoid. ⚠️ Therefore, ask the designers to see if it would be ok to leave it as it is and change their specifications. -> Seen in Spec meeting and it's ok to have this one.

      image

    • With a too long accordion title label, the accordion arrow disappears, as well as the ones of all the other accordion titles. The label should be truncated so that the arrow remains visible and a space shall be kept between the end of the (truncated) text and the accordion arrow.

      image

  • In mobile display:

    • The close button icon is not the correct one. (See Offcanvas: Change the close icon #1532)

      image

    • There are some extra vertical spaces around the examples. It would better without them supposing they can be reduced/removed

      image

    • Add in the doc that the side navigation menu can be situated on the right or on the left of the screen by using a specific css class .name_of_class

@louismaximepiton

This comment was marked as outdated.

@louismaximepiton louismaximepiton force-pushed the main-lmp-side-navigation branch from 1f9ca76 to ad4b8ce Compare August 23, 2022 09:13
@louismaximepiton louismaximepiton marked this pull request as ready for review August 23, 2022 09:13
@julien-deramond
Copy link
Contributor

Just tracking here a need for an Orange project to discuss with designers.
Be able to open the side menu just by hovering the image (could be an option: display the title OR opening the menu entirely)

@louismaximepiton louismaximepiton marked this pull request as draft November 30, 2022 15:44
@louismaximepiton
Copy link
Member Author

Just split PR with #2180 to ease the reviews.

@@ -22,6 +22,9 @@ If you need more details about the changes, please refer to the [v5.3.1 release]
- **Nav and tabs**
- <span class="badge bg-success">New</span> Added support for JavaScript Tabs' <kbd>Home</kbd> and <kbd>End</kbd> keys.

- **Side navigation**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to move this part to v5.3.x when ready 😉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I let it there for now since I don't know when this component will be merged. I'll move it once I know it!

Copy link
Contributor

@MewenLeHo MewenLeHo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small point to clarify on focus.

Copy link
Contributor

@MewenLeHo MewenLeHo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part looks good since the drawer content variant has been moved to a distinct PR.

@Aniort
Copy link
Contributor

Aniort commented Aug 4, 2023

good to me 👍
just add a tabindex=-1 on the ul element of the coolapsible nav to avoid the FF keyboard behavior on overflow content

@Franco-Riccitelli
Copy link
Member

Level 3 Label position
Move the labels at the 3rd level, 2 pixels to the left.
Label position

Arrow size/position
Reduce the size of the horizontal arrow to: Width 12px, Height 6px
I have attached the positioning of this arrow. See attached image.
The size and position of the vertical arrow (that reduces the width of the side navigation) is ok, no need to change that one.

Arrow position

Scroll bar
Is it possible to include the scroll bar inside the width of the menu without increasing the width of the menu?
See the attached example. This would also mean positioning the arrow (on the right) 20px from the right edge.

Scroll bar

Selection state
When drilling down the menu, the selection state should only remain on the label that represents the current page selection. Please see the attached example as a reference.

Side Navigation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Need Design and/or Accessibility Review
Development

Successfully merging this pull request may close these issues.

Navigation > Side Navigation
6 participants